-
-
Notifications
You must be signed in to change notification settings - Fork 359
add thomas algorithm in javascript #491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
||
thomas(a, b, c, x); | ||
|
||
for (let i = 0; i < 3; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use for (const value of x) {
here. Also, you could just print the array as a whole: console.log("has the solution: ", x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, went for the console.log("solution: ", x)
as it is simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As always, thanks for the contribution!
We now have a code style guide for JavaScript. You should check that out because there are a few ways your code deviates from it. The most obvious thing is that your code is indented with tabs instead of 2 spaces. The rest is explained in the comments.
Other than that, I just suggested to @leios on stream that we should probably make the implementations return the result instead of modifying the input. He already changed the Julia implementation on stream (#495).
@@ -0,0 +1,28 @@ | |||
function thomas(a, b, c, x) { | |||
let y = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be const
.
x[0] = x[0] / b[0]; | ||
|
||
for (let i = 1; i < a.length; i++) { | ||
let scale = 1.0 / (b[i] - a[i] * y[i - 1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also be const
.
You also don't have to use 1.0
. JavaScript doesn't differentiate between 1
and 1.0
.
everything corrected, @Butt4cak3 |
I'll take a look at it tomorrow. I didn't have access to my PC for most of today. |
You're still modifying the input ( We're still discussing whether we should actually make this a requirement (#510), but by the looks of it, we will. If you want, you can hold off with this PR until the issue is resolved or you can just go ahead and do it now. Your call. |
@Butt4cak3 , I corrected it just in case, if we then decide that pure functions shouldn't be compulsory and the code is more readable without a pure function I'll change it again, no prob. (I think it is more readable without it being pure but that's discussion for #510 :P) |
Just to clarify: I'm still waiting for that discussion to resolve. I didn't forget your PR. I just don't want us to have to do the work twice. |
We are coming up on one calendar year since the last comment. Is there anything left to be done? |
I'll stop trying to fool myself and others. I'm just not active enough in this community anymore. Any PRs that are currently assigned to me should be handled by someone else. I don't even know what these things are about anymore. |
This has been repurposed into #683 |
No description provided.